Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Paw/ct 1652 restore default logging #6447

Merged
merged 3 commits into from
Dec 14, 2022
Merged

Paw/ct 1652 restore default logging #6447

merged 3 commits into from
Dec 14, 2022

Conversation

peterallenwebb
Copy link
Contributor

@peterallenwebb peterallenwebb commented Dec 14, 2022 •
edited by dbeatty10
Loading

resolves #6434

Description

Restore logging to stdout before the logging subsystem is fully configured, so that earlier errors are still presented to the user in some form.

Checklist

@peterallenwebb peterallenwebb requested review from a team as code owners December 14, 2022 20:55
@cla-bot cla-bot bot added the cla:yes label Dec 14, 2022
@peterallenwebb
Copy link
Contributor Author

Verified locally the same behavior documented in the issue, and also verified that the propose fix restores the expected behavior:

(env310) ~/Code/dbt-labs/internal-analytics# dbt run
20:42:52  Error importing adapter: No module named 'dbt.adapters.snowflake'
20:42:52  Encountered an error while reading profiles:
20:42:52    ERROR: Runtime Error
  Credentials in profile "garage-snowflake", target "dev" invalid: Runtime Error
    Could not find adapter type snowflake!
20:42:52  Defined profiles:
20:42:52   - default
20:42:52   - peter_test_1
20:42:52   - garage-snowflake
20:42:52   - jaffle_shop
20:42:52
For more information on configuring profiles, please consult the dbt docs:

https://docs.getdbt.com/docs/configure-your-profile

20:42:52  Encountered an error:
Runtime Error
  Could not run dbt

@peterallenwebb
Copy link
Contributor Author

@MichelleArk You deleted your comment while I was responding, but yes, there is at least the danger that a logger could be double-added. This was my comment:

Our implementation of setup_event_logger() clears the loggers and handlers for this reason, before adding the configured loggers. My feeling is that anyone else who accesses EVENT_MANAGER directly is taking their life in their hands and should know what they are doing. We also hope to get rid of all globals associated with logging, and several other inelegant hacks in this file, in version 1.5. Hopefully we can discuss that further at Brown Bag.

@peterallenwebb peterallenwebb merged commit 7e90e06 into main Dec 14, 2022
@peterallenwebb peterallenwebb deleted the paw/ct-1652-restore-default-logging branch December 14, 2022 22:04
MichelleArk
MichelleArk reviewed Dec 14, 2022
View reviewed changes
core/dbt/events/functions.py
EVENT_MANAGER: EventManager = EventManager()
EVENT_MANAGER.add_logger(
_get_logbook_log_config() if flags.ENABLE_LEGACY_LOGGER else _get_stdout_config()
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[answered here] Is there a risk of double-adding loggers downstream, leading to extraneous calls to logger.write?

@jtcohen6 jtcohen6 mentioned this pull request Jan 3, 2023
Closed
2 tasks
@jtcohen6 jtcohen6 mentioned this pull request Jan 26, 2023
Closed
2 tasks
@iknox-fa iknox-fa mentioned this pull request Jan 30, 2023
Closed
6 tasks
@dbeatty10
Copy link
Contributor

This originally resolved #6436 on accident. I just updated it to #6434 instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichelleArk MichelleArk MichelleArk approved these changes

@emmyoop emmyoop emmyoop approved these changes

@mikealfare mikealfare Awaiting requested review from mikealfare mikealfare was automatically assigned from dbt-labs/core-group

Assignees
No one assigned
Labels
cla:yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[CT-1652] [Regression] [main/1.4] Failure to raise initialization exception
4 participants
@peterallenwebb @dbeatty10 @emmyoop @MichelleArk